[tool] Extract most conformance checks to separate validator classes#11610
[tool] Extract most conformance checks to separate validator classes#11610stuartmorgan-g wants to merge 6 commits intoflutter:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the repository's validation logic by extracting validation rules into dedicated validator classes for Dependabot, Gradle, pubspec, README, repository information, and version/changelog checks. It also introduces a utility function for generating POSIX-compliant relative paths. The changes improve code modularity and maintainability by separating validation logic from command execution.
| return succeeded; | ||
| } | ||
|
|
||
| /// Validates that [gradleLines] reads and uses a artifiact hub repository |
There was a problem hiding this comment.
There is a typo in the documentation comment: "a artifiact" should be "an artifact".
| /// Validates that [gradleLines] reads and uses a artifiact hub repository | |
| /// Validates that [gradleLines] reads and uses an artifact hub repository |
References
- Code should follow the relevant style guides for each language, which includes correct spelling and grammar in documentation. (link)
| } | ||
| '''; | ||
|
|
||
| /// Validates that [gradleLines] reads and uses a artifiact hub repository |
There was a problem hiding this comment.
There is a typo in the documentation comment: "a artifiact" should be "an artifact".
| /// Validates that [gradleLines] reads and uses a artifiact hub repository | |
| /// Validates that [gradleLines] reads and uses an artifact hub repository |
References
- Code should follow the relevant style guides for each language, which includes correct spelling and grammar in documentation. (link)
| ''' | ||
| $buildGradleName "namespace" must match the "package" attribute in AndroidManifest.xml, if one is present. | ||
| $buildGradleName namespace: "$namespace" | ||
| AndroidMastifest.xml package: "${packageMatch.group(1)}" |
There was a problem hiding this comment.
There is a typo in the error message: "AndroidMastifest.xml" should be "AndroidManifest.xml".
| AndroidMastifest.xml package: "${packageMatch.group(1)}" | |
| AndroidManifest.xml package: "${packageMatch.group(1)}" |
References
- Code should follow the relevant style guides for each language, which includes correct spelling in user-facing messages. (link)
| if (implements == null) { | ||
| return 'Missing "implements: $expectedImplements" in "plugin" section.'; | ||
| } else if (implements != expectedImplements) { | ||
| return 'Expecetd "implements: $expectedImplements"; ' |
There was a problem hiding this comment.
There is a typo in the error message: "Expecetd" should be "Expected".
| return 'Expecetd "implements: $expectedImplements"; ' | |
| return 'Expected "implements: $expectedImplements"; ' |
References
- Code should follow the relevant style guides for each language, which includes correct spelling in user-facing messages. (link)
|
|
||
| String _issueTagForPackage(String packageName) { | ||
| // TODO(stuartmorgan): Move this to a config file. See | ||
| // https://github.com/flutter/flutter/issues/185364 |
There was a problem hiding this comment.
The issue link in the TODO appears to have a typo. Issue 185364 does not exist in the flutter/flutter repository, whereas issue 155364 specifically discusses moving issue labels to a config file, which matches the context of this TODO.
| // https://github.com/flutter/flutter/issues/185364 | |
| // https://github.com/flutter/flutter/issues/155364 |
| } | ||
|
|
||
| test('skips with no supported ecosystems', () async { | ||
| test('passes with no supported ecosystems', () async { |
There was a problem hiding this comment.
Skipping won't make sense in the combined command (no package skips all of the checks that will be in the combined command), so I didn't restructure the common validator pattern to allow it just for this command, only to remove it again in the next PR, and instead changed this behavior now.
There was a problem hiding this comment.
All the changes in this file are just because the constants moved.
| p.split(p.relative(f.absolute.path, from: root.path)), | ||
| ), | ||
| (File f) => | ||
| relativePosixPath(f, from: root, platformContext: p.posix), |
There was a problem hiding this comment.
Since I was extracting a helper for use in the validators (to replace the one in the looping command), I changed this to use the new helper that has the same logic.
| 'Allowing breaking change to plugin_platform_interface due ' | ||
| 'to --ignore-platform-interface-breaks', | ||
| 'Ignoring breaking change to plugin_platform_interface due ' | ||
| 'to command configuration', |
There was a problem hiding this comment.
I didn't want to pass in the flag name to the validator just to keep this message the same. I don't think having the message be less descriptive is going to matter for a flag that's only run in CI, and in a message that's just for auditing if something weird ever happens. We'll still be able to trace the message back to this line.
1 participant
Extracts the logic for many of the lightweight, repo-practice commands into separate validator classes, leaving just a minimal amount of logic in the command, in preparation for combining all of these into a single command. Doing so will make running conformance checks locally much easier since there will be fewer commands to run, and will make documenting both the tool itself, and the commands we want run against PRs (e.g., for agent instructions), easier as well.
This is a mostly no-op code move, so that tests can change as little as possible in this PR. Then the next PR will then combine the commands, which will require a lot of test changes, but almost all of the non-test code will be able to stay unchanged because it's in the separate validator classes, which the combined command can call into as they are.
All the new files were created by moving code from the corresponding command file, with very minimal changes (e.g., sometimes context that was a non-private getter from the base command class is a private field in the validator class, so underscores were added.) This is reflected in the fact that there are almost no test changes.